refactor(worker-javascript): extract Phase 2 helpers from Core.js#1038
Merged
dqnykamp merged 8 commits intoDoenet:mainfrom May 3, 2026
Merged
refactor(worker-javascript): extract Phase 2 helpers from Core.js#1038dqnykamp merged 8 commits intoDoenet:mainfrom
dqnykamp merged 8 commits intoDoenet:mainfrom
Conversation
Begin breaking up the 13,837-line Core class by lifting seven self- contained, low-coupling helpers into TypeScript modules. The pattern matches the existing composed siblings (Dependencies.js, ParameterStack): each module is constructed with a `core` back-reference, and Core retains a thin delegating wrapper for every method/property that was previously on the class so external callers (CoreWorker, tests, components, and `coreFunctions`-bound references) continue to work unchanged. Modules extracted: - DiagnosticsManager.ts — diagnostics queue + source-location walk - StateVariableNameResolver.ts — pure-function name resolution utilities - VisibilityTracker.ts — visibility state and save/suspend timers - StatePersistence.ts — save to localStorage / database - AutoSubmitManager.ts — debounced answer-submit queue - NavigationHandler.ts — handleNavigatingToComponent, navigateToTarget - ResolverAdapter.ts — adapter to the external Rust name resolver No behavior change. Core.js drops from 13,837 to 12,909 lines. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Continues the Core.js breakup with six moderate-effort modules. Same pattern as Phase 1 (composed sibling holding a `core` back-reference, thin delegating wrapper on Core for every public method/property). No behavior change. Modules extracted: - RendererInstructionBuilder.ts — owns componentsToRender, componentsWithChangedChildrenToRender, rendererState; the dast instruction stream sent to the renderer - ProcessQueue.ts — owns processQueue, processing, stopProcessingRequests; async request queue and entry-point scheduling (executeProcesses, requestAction, requestUpdate, requestRecordEvent) - ComponentLifecycle.ts — stateless: registration, ancestors, defining-child splicing, propagation to shadows - ChildMatcher.ts — child-group matching, adapter substitution, rendered-child filtering (recursion guard only) - DeletionEngine.ts — stateless two-phase component deletion - ActionTriggerScheduler.ts — owns stateVariableChangeTriggers, actionsChangedToActions, originsOfActionsChangedToActions; trigger polling and chained-action graph Core.js drops from 12,909 to 11,253 lines (this PR), 13,837 → 11,253 since the refactor began (~18.7%). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
This was referenced May 1, 2026
# Conflicts: # CLAUDE.md # packages/doenetml-worker-javascript/src/AutoSubmitManager.ts # packages/doenetml-worker-javascript/src/Core.js # packages/doenetml-worker-javascript/src/DiagnosticsManager.ts # packages/doenetml-worker-javascript/src/ResolverAdapter.ts # packages/doenetml-worker-javascript/src/StatePersistence.ts # packages/doenetml-worker-javascript/src/StateVariableNameResolver.ts # packages/doenetml-worker-javascript/src/VisibilityTracker.ts
Replaces the open-coded three-field reset in `Core.generateDast` with `processQueueManager.reset()`, matching the pattern used by `RendererInstructionBuilder` and `ActionTriggerScheduler`. Also updates `CORE_REFACTOR_DEFERRED.md` to extend the `core: any` typing item and the stateless-managers item with the Phase 2 managers (`RendererInstructionBuilder`, `ProcessQueue`, `ComponentLifecycle`, `ChildMatcher`, `DeletionEngine`, `ActionTriggerScheduler`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Documentation and small cleanups in the Phase 2 managers, plus deferred items captured for follow-up: - RendererInstructionBuilder: rewrite the misleading "(slated for later phases)" docstring (deriveChildResultsFromDefiningChildren and returnActiveChildrenIndicesToRender are in ChildMatcher this PR) - All six new managers: add method-level docstrings on the non-obvious public methods; the top-of-module docs already covered ownership - ProcessQueue: extract `_kickoff()` (was duplicated 3x in the request entry points), and attach `.catch(console.error)` to its `executeProcesses()` invocation per AGENTS.md - ChildMatcher: convert `derivingChildResultsInProgress` from number[]-as-Set to a real `Set<number>` - ComponentLifecycle: drop redundant `expandComposites: true` in the retry call (it's the default), drop `recursive === true` - DeletionEngine: rewrite the broken-numbering "1./3./4." comment as a proper two-phase docstring; rename `compositeIdxStr` to `compositeIdx` (it's a number, not a string) - All managers: standardize on `core._components` (was mixed with `core.components` getter, same array) Deferred to CORE_REFACTOR_DEFERRED.md: - Sweeping remaining fire-and-forget calls outside ProcessQueue - Carried-over TODO/XXX markers (line numbers updated for new files) - Renaming `processQueueManager` to `processQueue` (requires also dropping Core's `get/set processQueue` accessor that exposes the underlying array) - The `_components`/`components` access convention (now consistent in Phase 2; document so future managers follow suit) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dqnykamp
added a commit
to dqnykamp/DoenetML
that referenced
this pull request
May 3, 2026
Brings in the squash-merged Phase 1 (Doenet#1036) and Phase 2 (Doenet#1038) PRs along with their post-review changes, and adopts the same review-driven patterns in the remaining Phase 3 modules. Phase 1/2 manager files (ActionTriggerScheduler, AutoSubmitManager, ChildMatcher, ComponentLifecycle, DeletionEngine, DiagnosticsManager, ProcessQueue, RendererInstructionBuilder, ResolverAdapter, StatePersistence, StateVariableNameResolver, VisibilityTracker) plus AGENTS.md, CLAUDE.md, and the new utils/timerErrors.ts and CORE_REFACTOR_DEFERRED.md are taken from upstream. Core.js merges Phase 3's extraction wrappers with upstream's Phase 1/2 review changes: - import * as nameResolver (replaces aliased named imports) - short-form `// → managerName` markers replace the long per-section comment blocks - top-of-class block comment now lists all phases - removed `set hasPendingDiagnostics` (callers go through markPending()) - replaced open-coded processQueueManager three-field reset with processQueueManager.reset() - added `// → componentBuilder`, `// → compositeExpander`, `// → stateVariableDefinitionFactory`, `// → stateVariableInitializer`, `// → resolverAdapter` markers for the new sections Phase 3 modules adopt the Phase 2 review conventions: - ComponentBuilder.ts: `core.hasPendingDiagnostics = true` → `core.diagnosticsManager.markPending()` (the setter on Core was removed in the Phase 1 review) - ComponentBuilder.ts and CompositeExpander.ts: standardize on `core._components` (was using the `core.components` getter for the same underlying array) CORE_REFACTOR_DEFERRED.md updates: - Type-the-`core: any` and stateless-managers items now list Phase 3 managers alongside Phase 1/2. - Pre-existing fire-and-forget Core.js line numbers updated for the post-Phase-3 layout (444, 4387, 483-486). - Carried-over TODO/XXX inventory now includes the Phase 3 markers in StateVariableInitializer, StateVariableDefinitionFactory, ComponentBuilder, and CompositeExpander. - _components/components convention note now covers Phase 3. Verification: - `tsc --noEmit` produces strictly fewer errors than the pre-merge backup (359 vs 365 — the removed setter and the `core._components` switch resolve a handful of `any` complaints; nothing new was introduced). - callAction.test.ts (20/20 — exercises StatePersistence + ProcessQueue + ActionTriggerScheduler), circle.test.ts and spreadsheet.test.ts (65 passed, 1 todo — exercises StateVariableInitializer's array-callback path that the Phase 3 follow-up commit fixed) all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 2 of breaking up
packages/doenetml-worker-javascript/src/Core.js. Same pattern as Phase 1 (composed sibling holding acoreback-reference, thin delegating wrapper on Core for every public method/property). No behavior change.Net effect over both phases:
Core.jsdrops from 13,837 → 11,253 lines (-18.7%).Phase 1 (#1036) is now merged.
upstream/mainhas been merged into this branch so the diff againstmainshows only Phase 2 changes plus the// → managerNamecomment-style alignment described below.Modules extracted in Phase 2
RendererInstructionBuilder.tscomponentsToRender,componentsWithChangedChildrenToRender,rendererState; the dast/instruction stream sent to the rendererProcessQueue.tsprocessQueue,processing,stopProcessingRequests; the async request queue and entry-point scheduling (executeProcesses,requestAction,requestUpdate,requestRecordEvent)ComponentLifecycle.tsregisterComponent,deregisterComponent,setAncestors,processNewDefiningChildren,spliceChildren,addChildrenAndRecurseToShadowsChildMatcher.tsDeletionEngine.tsActionTriggerScheduler.tsstateVariableChangeTriggers,actionsChangedToActions,originsOfActionsChangedToActions; trigger polling and chained-action graphSequencing rationale
Per the multi-phase plan:
RendererInstructionBuilderandProcessQueuefirst (highest readability per line, unblock downstream);ChildMatcherandDeletionEngineare larger but algorithmically isolated;ActionTriggerSchedulerlast because it sharesupdateInfofields with the perform bodies still in Core.Carry-over from Phase 1 review
When
upstream/mainwas merged into this branch, the Phase 1 review changes were preserved and the same conventions were extended to the Phase 2 wrappers:Core.jsuse the// → managerNamemarker style introduced by Phase 1's review (replaces the longer multi-line "X lives in Y" comments).hasPendingDiagnosticsdirectly (so Phase 1's removal of theset hasPendingDiagnosticsaccessor doesn't break them) and don't introduce anysetTimeout/.then()chains that would need the newreportTimerErrorhelper.Deferred follow-ups
The deferred items in
CORE_REFACTOR_DEFERRED.md(added by Phase 1) now also apply to Phase 2 — most notably:core: anyback-reference in the new managers (RendererInstructionBuilder,ProcessQueue,ComponentLifecycle,ChildMatcher,DeletionEngine,ActionTriggerScheduler) — same shape as the Phase 1 managers.ComponentLifecycle,ChildMatcher, andDeletionEngineare stateless (only hold acoreback-reference) and would fit the pure-function shape used inStateVariableNameResolver.ts.Both are intentionally out of scope for this PR — better tackled as a single follow-up over both phases.
Test plan
npm run build -w @doenet/doenetml-worker-javascriptpassesnpm run build -w @doenet/doenetmlpassestsc --noEmitclean for@doenet/doenetml-worker-javascriptRendererInstructionBuilder(renderer state delivery includes diagnostics)ChildMatcher(alias/composite paths) and the new manager↔core back-refsActionTriggerScheduler(chained actions on submitAnswer)🤖 Generated with Claude Code